memorable: optional procedural memory, off by default - #872
Open
NIkhil-cmd-cmd wants to merge 12 commits into
Open
memorable: optional procedural memory, off by default#872NIkhil-cmd-cmd wants to merge 12 commits into
NIkhil-cmd-cmd wants to merge 12 commits into
Conversation
Gives QM procedural memory: a finished session's tool-call trace is
parsed — deterministically, no model involved — into a procedure
recording which files changed and what verified the work, stored in
QM's own Postgres. A later session on a similar task gets a short
pointer appended to its prompt, so it goes straight to the fix instead
of re-deriving the diagnosis.
Five concerns, previously five stacked PRs, combined here:
1. src/memorable/capture.ts — session entries to tool-call records.
2. Tool outcomes joined onto those calls by callId: ok from isError
(the one universal flag), exit codes only where execute really
records them. Never inferred.
3. README section describing the integration and its defaults.
4. src/memorable/relay.ts — when the last run for a thread finishes,
the capture is piped to `memorable record`. The CLI owns
extraction, consent, and storage; QM never talks to the network or
to the memorable tables itself.
5. src/memorable/inject.ts — the incoming task is offered to
`memorable inject`; a hit is appended after the memory block, past
the prompt-cache boundary. A miss injects nothing.
Off by default: MEMORABLE=1 enables, QM_MEMORABLE=0 is the kill-switch,
MEMORABLE_BIN names the binary — all parsed once in loadConfig per the
config-boundary rule. Per-scope consent must be explicitly read-write
before anything persists.
Failures can't touch a turn: the relay is fire-and-forget with every
error swallowed and the child unref'd; injection has a hard timeout and
drops any output lacking the data-not-instructions envelope, strips
control characters, and caps size.
Verified: typecheck, prettier, eslint, knip, and the three memorable
test files (11 tests) all clean.
Recall can now answer a prompt that is several things at once with a plan — several stored procedures in dependency order rather than one pointer. The block uses the same envelope and the same size cap, so the adapter needs no change to accept one; the README says what a plan is and how to ask for it (`memorable inject --chain`, or MEMORABLE_CHAIN=1, opt-in per call). One real fix falls out of it. The adapter sliced an over-length injection to fit the 8000-character cap, and the guardrail that marks the block as inert data sits at the END of it — so truncating removed exactly the sentence that makes injection safe. A single pointer never came close to the boundary; a multi-step plan can. Over-length is now dropped, like every other malformed injection, and two tests pin it.
… CSI The adapter's stripper matched CSI as a whole sequence and let every other family fall through to a character class containing \x1b, so the escape was deleted and its argument reached the prompt as text: \x1b]0;pwned\x07 arrived as ]0;pwned. This runs on a subprocess's stdout on its way into the model's context, so it is the last thing between untrusted output and the prompt. CSI, OSC, DCS/SOS/PM/APC and two-character escapes are each matched whole now, and the test asserts over seven families that no control byte and no payload survives.
A lone escape took the character after it out of the injected block. Real sequences still go whole; a stray ESC now takes only itself.
…ering Four things an upstream review would have bounced. QM_MEMORABLE was compared with `!== "0"`, the only boolean in loadConfig that bypassed boolEnvStrict. Measured: false, off, no, FALSE and none all left the feature running, and =2 threw nothing where every other flag does. An operator reaching for a documented kill switch during an incident types false as readily as 0, sees no error, and believes it is off. It now parses like every other flag, and the matrix lives in test/config.test.ts beside them rather than in the relay's own test file, which is where it would have been caught. The relay re-offered a session's whole workflow list on every terminal run, and dedupe keys on a stored id, so a workflow the judge refuses is never stored and is therefore re-offered forever. Measured against the live service: 8 extract calls for 3 workflows in a 4-prompt session, which is O(N squared) in a session the gate rejects. QM holds no state, so it cannot remember a refusal; instead it no longer offers what the deterministic prefilter is certain to refuse. Fewer than two tool calls cannot produce two steps, and an all-identical run collapses into one. Both are one-way implications of refusal, so nothing that could be stored is dropped, and the filter sits in relayRecord rather than captureSession so the splitter contract is untouched. wiring.ts read the whole session log on every relay. The store already takes sinceSeq and compaction.ts already uses it; the relay now does too. That was quadratic in the host's own process, not only in our billing. inject.ts carried 25 comment lines in 83 against a repo baseline under one percent, and three of the blocks narrated a bug from an earlier commit in this same stack that never existed upstream. That is commit-message content by this repo's own standard. The one-line regex labels stay. Also: the relay timer is unref'd so a hung run cannot pin the event loop for two minutes, a duplicate empty-workflow guard is gone, .env.example documents the three knobs README line 199 promises are documented in place, and an ADR is included because CONTRIBUTING.md asks for prose before code.
…pair The write path capped the prompt at 16,000 characters and ran it through the terminal-control stripper; the read path did neither, so an 8MB paste produced an 8MB stdin write to the recall child on the turn's critical path, measured. Both paths now share one bound and one stripper. Both caps sliced by UTF-16 code unit, so a cap landing inside a surrogate pair left a lone high surrogate that JSON.stringify escapes as \ud83d. Postgres rejects that in a JSON literal and replaces it through a bound parameter. clampChars drops the orphan half.
…t was fixed docs/procedural-memory.md still documented QM_MEMORABLE as the literal value 0, which was the defect the previous pass repaired; the parser now takes the whole boolean vocabulary. It also counted the dormant modules at 155 lines when they are 221. Both variables are read once at boot and nothing said so. The README pointed at a repository that answers 404 to anyone outside this account.
…he capture
A flagged input is appended as a user entry carrying { text, securityTainted: true,
hidden: true } and the turn returns pending_approval, so the model never sees it and
historyHasSecurityTaint resets the harness session. captureSession read every entry with
no taint filter, so at the next terminal run that quarantined text became a workflow
prompt, went to the extraction child, and could come back injected into a later system
prompt. Tainted entries are now skipped, and a tainted user entry still closes the open
workflow so following calls are not attributed to the previous prompt.
forModelContext is the existing helper for this predicate but it also truncates at the
latest context summary, which would silently drop capturable work.
The relay called a vendored fork of the Memorable CLI that shipped inside the eval repo and inherited none of its fixes. It now calls the published binary, one array element different: record --scope <id> -, reading the capture on stdin the way the rest of that CLI reads stdin.
…in your database The integration doc handed an operator a MEMORABLE_API_KEY row and never said where a key comes from. There is no form to fill in: 'memorable login' creates the account and writes the key. A server has no browser, so the doc now says to copy it out of the config file into the environment. It also claimed nothing but the two spawns. The CLI's qm backend creates memorable_procedures, memorable_mode and memorable_stats in the database DATABASE_URL already points at. QM ships no migration for them, but they do appear, and an operator should read that here rather than find it in psql.
…d one One key per deployment was the wrong shape for a multi-tenant harness. Every scope's procedures landed in one Memorable organization, and whoever held that key could read all of them. Each scope can now connect its own account through a device authorization (RFC 8628). QM asks the sign-in service for a code, hands the human a URL, and stores whatever key comes back. QM never sees a password and cannot create an account for someone who has not signed in themselves. The key a spawn uses resolves scope, then org, then the environment, so an operator who connects the org scope once answers for every channel under it, a team that connects its own scope overrides that for itself, and a deployment that connects nobody behaves exactly as it did before. Stored keys are encrypted at rest under deriveConnectorKey(CONNECTOR_SECRET_KEY, "memorable-accounts"), the same AES-256-GCM path model_credentials uses. Without that key material the account store is not built at all, rather than writing a bearer token to Postgres in the clear. A row that will not decrypt reads as no key rather than throwing. This adds the one outbound call the integration otherwise avoids, to two device endpoints that carry an opaque code and a label like "qm channel a1b2c3d4" — the scope kind and a truncated hash, never the scope id, never a prompt or a tool call. The fetch is injected, so no test here reaches the network. The docs now describe that split instead of claiming QM makes no call at all, and say what the two new artifactMap tables are. Spawned children get an allow-listed environment built once in loadConfig, mirroring codexProcessEnv and claudeProcessEnv, rather than the whole process environment they inherited before. Also drops the comments this integration had been carrying, per the repo's zero-comment standard, and the unused RELAY_TIMEOUT_MS export knip was flagging.
… is its own act Review found the integration did not work end to end and could leak a key. Nothing was ever stored. `memorable record -` checks consent before it writes and exits 3 with memorable_write_denied; consent starts unset, connecting an account does not set it, and there is no terminal on a server to run `memorable enable`. The relay swallowed the exit code, so a person would connect, see "connected", and have every capture refused in silence. There is now a consent route that spawns the CLI verb for the caller's scope, the relay reads the child's stdout and returns an outcome, and a refusal lands in the error log as memorable_relay_refused rather than nowhere. The spawned CLI also only got MEMORABLE_BACKEND=qm if an operator happened to set it, and .env.example never mentioned it, so procedures would have gone to a per-machine file instead of this deployment's Postgres. QM sets the backend and MEMORABLE_DB_URL itself now, and no longer forwards DATABASE_URL at all: the child gets the connection string under the one name meant for it. A disconnect racing an in-flight approval left a live key behind. poll() read the pending record, spent up to ten seconds on the network, then wrote the account without rechecking, so a DELETE in that window reported nothing disconnected while the key was persisted anyway. poll() now claims the row with DurableMap.take() before writing, which also fixes two concurrent polls telling one caller the login failed when it had succeeded. Connects are self-only. authorizeAdmin ignores its scope argument, so it was a global admin check, and these routes sit outside /v1/admin/ where the live-actor and DM guards run: an injected prompt in an admin's cron turn could have bound a victim's scope to an attacker's organization. connectors.ts already refuses the identical operation with no admin override, and this now matches it. The org fallback tier became unreachable as a result and is gone rather than left dead. Also: the relay honours memoryPolicy instead of only its own switch; an apiKey passed without an env no longer silently drops the key and inherits the whole process environment; the stored key is bounded rather than truncated at 200 characters into something corrupt; a 5xx or 429 from the token endpoint reports unavailable and keeps the code alive instead of reading as "waiting for approval" for ten minutes; a 4xx retires it; and a second start() hands back the code the person is already looking at rather than orphaning it. Docs carry the parts that stayed true and the parts that did not: the egress split, what lands in the database, why a shared channel uses the deployment key, and the gaps still open on redaction, rate limiting and device-code sweeping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gives QM procedural memory. When a session ends, its tool-call trace is parsed,
deterministically, with no model in the pipeline, into a procedure recording
which files changed and what verified the work. A later session on a similar
task gets a short pointer appended to its prompt and goes straight to the fix
instead of re-deriving the diagnosis.
Rebased onto current
main, 0 deletions. Supersedes the five stacked PRs(#1, #2, #3, #4, #5).
Three switches, and none of them is on
MEMORABLE=1enables the integration;QM_MEMORABLE=0is the kill switch thatwins over everything. Both are parsed once in
loadConfig, per theconfig-boundary rule. The relay also honours QM's own
memoryPolicy: withcapture off no session-end hook is registered, and with recall off no injection
is attempted, so procedural memory cannot outlive the switch operators already
have.
Then, separately, connecting is not consent. Nothing is captured for a scope
until someone sets it to
read-write. Two absent answers areunset, andunsetis deny.Each person connects their own account
One key per deployment is the wrong shape for a multi-tenant harness: every
scope's procedures land in one organization, and whoever holds that key reads
all of them.
So a person connects their own, through a device authorization (RFC 8628). QM
asks the sign-in service for a code, hands them a URL, and stores whatever key
comes back, encrypted. QM never sees a password and cannot create an account for
someone who has not signed in themselves.
These act on the caller's own
personal:<actorId>scope and nothing else.Naming any other scope is refused, with no admin override, for the same reason
connectors.tsrefuses it: whoever opens the URL is whoever the key belongs to,so binding it to a channel would file the first person who clicked under
everyone else's name, and binding it to another person's scope would let an
admin route their transcripts to an organization they do not control.
The consequence, which is deliberate: a session in a shared channel runs under
channel:<id>, which nobody can connect, so it uses the deployment's own key.Per-person accounts cover personal-scope sessions. A key is never borrowed by a
scope that did not connect it.
Credentials
Stored keys are encrypted at rest with
deriveConnectorKey(CONNECTOR_SECRET_KEY, "memorable-accounts"), the sameAES-256-GCM path
model_credentialsuses. Without that key material the accountstore is not built at all, rather than writing a bearer token to Postgres in the
clear. A row that will not decrypt reads as no key rather than throwing.
Spawned children get an allow-listed environment built once in
loadConfig,mirroring
codexProcessEnvandclaudeProcessEnv.DATABASE_URLisdeliberately not in it: the child gets the connection string only as
MEMORABLE_DB_URL, the one name meant for it.MEMORABLE_BACKENDdefaults toqmso the CLI cannot be silently misconfigured into writing a per-machine fileinstead of your Postgres.
Egress, stated plainly
An earlier revision of this branch claimed QM makes no network call of its own.
With accounts that is no longer true, and the docs say so rather than burying
it. QM calls two endpoints, only when someone starts a connect:
POST <api>/v1/device/codeqm personal a1b2c3d4: the scope kind and a truncated hash, never the scope idPOST <api>/v1/device/tokenNeither carries a prompt, a tool call, a file, or a transcript.
<api>isMEMORABLE_API_URL, so you can point it at your own deployment. Thefetchisinjected, so no test here reaches the network. Recall and capture stay
spawn-only.
Where the data goes
Five tables on QM's DurableMap row shape, in the database
DATABASE_URLalreadypoints at. No new database and no migration: three are the CLI's, created on
first write, and two are QM's own, created lazily by
artifactMapexactly asconsent_linksandsecret_dropsare.Pulling the integration out leaves all five behind; drop them if you want the
data gone.
docs/procedural-memory.mdsays this rather than leaving it to befound in
psql.What review changed
Two independent reviewers went at this with no part in writing it. Everything
below is a bug they found and this branch fixes, with a test each.
poll()read the pending record, spent up to ten seconds on the network, then wrote the account without rechecking. ADELETElanding in that window reporteddisconnected: falseand the key was persisted anyway, so that scope's transcripts kept flowing to an org nobody thought was connected.poll()now claims the pending row withDurableMap.take()before writing, and a lost claim reports the truth instead.authorizeAdminignores its scope argument, so it is a global admin check, and these routes sit outside/v1/admin/where the live-actor and DM guards run. An injected prompt in an admin's cron turn could have rebound a victim's scope. Now self-only, matching whatconnectors.tsalready decided for the identical operation.memoryPolicy. A deployment running capture off still relayed full transcripts.DATABASE_URLwent to a third-party binary on every turn, alongside a redundantMEMORABLE_DB_URL. Removed.apiKeywithoutenvsilently dropped the key and inherited the whole process environment. The type no longer allows it.expired. Now 5xx and 429 reportunavailableand keep the code alive; a 4xx retires it.start()orphaned the code the person was already looking at. It now hands back the live one.Reviewers also confirmed clean: the encryption purpose and ordering, no key in
any response, audit record, log or catalog, the device-code label, spawn argv
safety, and that RAM-backed maps without
DATABASE_URLare the repo-widepattern rather than something this branch introduced.
Known gaps
captureSessionforwards tool inputs and the prompt, capped, through the CLI's own minimization. QM does not runcreateSecretValueMaskerover them. A credential pasted into a tool argument would travel. This should be settled before anyone turns capture on for a scope that handles secrets, and I would rather agree the approach with you than guess at it.memorable_device_codes. A scope that starts a sign-in and never polls leaves its row until it does.auth: "either".Verification
Typecheck, eslint, prettier and knip clean.
npm testgreen.79 tests across the six memorable files. The ones that would have caught the
list above: a disconnect racing an in-flight approval leaves no key; two
concurrent polls both report the truth; a 502 keeps the code and a 400 retires
it; an over-long or missing key stores nothing; a key is never borrowed by
another scope; the stored row never carries the key in the clear; a row under
different key material reads as no key rather than crashing; and every connect
route refuses a scope that is not the caller's, in the body and in the query
string.
The device flow was also round-tripped against the live service using
accounts.tsitself rather than a stub:startreturned a real approval URL,pollreturnedpending,keyForreturned null until approval.The red checks on this PR are the fork, not the branch. This mirror has no
Actions runners, so every job fails in three to five seconds with no steps and
no runner assigned. Unmodified upstream
main, pushed to this same mirror,fails identically.
Numbers
Measured on QM's own bench, codex harness, local docker sandbox, live
extraction path, pass decided by an independent in-sandbox
sh test.shrun bythe harness rather than the agent's claim.
Tool calls drop 40%, pooled over three independent replications at n=25 per
arm (150 runs), Mann-Whitney z=4.06, p=0.00005, same direction in all three.
Pass rate was 91% injected against 80% baseline pooled, ahead in every
replication, but p=0.065, so treat it as directional and not established.
An earlier internal report quoted a 60% tool-call reduction plus input-token,
latency and output-token gains. Those did not replicate and are withdrawn.
The replication also found the eval's actors were fixed strings, so both arms
shared one durable workspace across campaigns and a later baseline opened onto
an earlier run's solved fixtures. Fixed with a per-replication actor tag. The
40% figure is the number measured after that fix, and it is the only one we
quote.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.